Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-generate TypeScript Typings from Documentation #38

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jvilk
Copy link

@jvilk jvilk commented Feb 8, 2016

I am opening this PR to start a conversation. It should not be merged as-is until we determine how to integrate these typings into nodegit.

Summary

I have created a script that creates valid TypeScript typings for NodeGit from the documentation JSON. As NodeGit improves its documentation, the TypeScript typings will improve as well.

This is what the working typings look like.

Usage

Run generate/index.js in the usual manner. It will generate ts/nodegit.d.ts.

To create a new project that uses this file, do the following:

npm install -g typescript tsd
mkdir test_proj
cd test_proj
npm install nodegit
tsd install node
cp path/to/nodegit.github.com/ts/nodegit.d.ts node_modules/nodegit/lib/

Edit node_modules/nodegit/package.json to include a "typings": "lib/nodegit.d.ts" field. This tells the TypeScript compiler where to find typings for your package.

Create tsconfig.json in test_proj:

{
  "compilerOptions": {
    "target": "es3",
    "module": "commonjs",
    "noImplicitAny": true
  },
  "files": [
    "typings/node/node.d.ts",
    "test.ts"
  ]
}

Create the test.ts in test_proj:

import Git = require('nodegit');

var getMostRecentCommit = function(repository: Git.Repository) {
  return repository.getBranchCommit("master");
};

var getCommitMessage = function(commit: Git.Commit) {
  return commit.message();
};

Git.Repository.open("nodegit")
  .then(getMostRecentCommit)
  .then(getCommitMessage)
  .then(function(message) {
    console.log(message);
  });

Compile test.ts:

$ tsc

Then, you can fiddle with the code in an editor that supports TypeScript, such as Visual Studio Code or Atom with the atom-typescript plugin. Simply open up the test_proj folder, and the editor will use tsconfig.json to orient itself and provide autocomplete using the typings.

Limitations

Currently, there are a number of weird type annotations that I have to smooth over or type to the 'any' type (see the getType function). Most of them are untyped callback functions. As you improve NodeGit's documentation, we can remove some of the logic in that function.

Suggestions

It would be ideal if:

  • You included generated TypeScript declaration files in nodegit's NPM module, as illustrated in the example.
  • You added a test to your testsuite that verifies that the produced declaration file compiles properly. If it does not, it likely indicates a documentation bug!

If the script breaks, ping me. In the worst case, you can ship w/o decls for a release. In the best case, you'll fall in love with TypeScript and decide to maintain it yourselves! 😄

An alternative proposal is to put the types in the DefinitelyTyped repository, where they can be maintained separately. But I'd argue that including them into the NPM module directly would be mutually beneficial -- automatic checking of your docs, and typings for TypeScript users!

Thoughts?

John Vilk added 4 commits February 8, 2016 14:27
Currently, the declarations are not correct due to the incompleteness of the documented types.

I will need to fix up some documented types, and add hacks that remove certain annotations.
…ions.

There are now only a few issues left to deal with.
@jvilk
Copy link
Author

jvilk commented Feb 8, 2016

Oh, and I can remove my name from the generated typings. I just added most of the standard headers that you see in other manually curated TypeScript typings without thinking.

@jvilk
Copy link
Author

jvilk commented Feb 8, 2016

Updated sample file to use tsconfig.json, which is needed for editor support.

@jvilk
Copy link
Author

jvilk commented Feb 10, 2016

Updated with a link to the typings, in case someone finds this and wants to use them immediately!

@jvilk
Copy link
Author

jvilk commented Feb 10, 2016

Fixed generation to support optional parameters, and updated linked typings with the fix.

@jvilk
Copy link
Author

jvilk commented Feb 10, 2016

Aw, fiddlesticks. My last commit broken the typings because some functions have optional parameters followed by required parameters. TypeScript supports this via multiple function declarations for the same function, so I'll need to retool the script a little bit.

Here's the older, working typings with non-optional types. I'll fix the optional type bug in a few hours.

…arameter list by generating variants.

Handles complexities like Git.Commit.create.
@jvilk
Copy link
Author

jvilk commented Feb 10, 2016

Fixed optional parameters, so complex functions like Commit.create are specified properly using multiple declarations. Link to typings updated in PR description.

@johnhaley81
Copy link
Collaborator

Hey @jvilk sorry it's taken so long to get back to you. So my initial feeling about this is really good. Although we don't use TypeScript in GitKraken it's used in a lot of projects out there and if anything could benefit from type safety, it would be NodeGit.

So the main thing that stands out to me is grinding down this list. This stuff should be fairly straight-forward to generate (if the docs are good) and should require minimal changes as NodeGit/libgit2 evolves.

I really like the test idea to confirm that docs are good and I think that would be a must for this. NodeGit's biggest weakness is the docs and I think that would help with the overall health.

Anybody have any thoughts on this overall? /cc @tbranyen @maxkorp @Mr-Wallet @srajko @dkoontz

@jvilk
Copy link
Author

jvilk commented Feb 15, 2016

For testing, you'll need to modify nodegit's testing configuration such that it:

  • Clones this repository
  • Symlinks nodegit into the generate folder
    • ...which may be a problem on Windows. On some configurations, junction attempts (Windows symlinks) are met with a permissions error.
    • Alternatively, you could modify your doc generation script so you can configure nodegit's location to avoid the symlink.
  • Runs the nodegit.d.ts generator
  • Installs the TypeScript NPM module
  • Runs the TypeScript compiler on an empty ts file that references nodegit.d.ts

You'll also need to include node.d.ts, the Node TypeScript declarations, in your repository or fetch it at test time. I recommend the former option to reduce your tests' network dependencies.

For prepublish to NPM, you'll also need a script that does the above to generate nodegit.d.ts, and you'll need to add a typings field to the package.json.

@tbranyen
Copy link
Member

I'm gonna look into this either this weekend or next week! Thanks for making the PR @jvilk

@johnhaley81
Copy link
Collaborator

@tbranyen did you ever get a chance to look into this? I'd like to plop this in for v0.13.0.

type = type.slice(0, angleIndex) + '<' + getType(type.slice(angleIndex + 1, type.indexOf('>'))) + ">";
}

switch (type) {
Copy link
Member

@tbranyen tbranyen Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should eventually move this to a dedicated JSON file like the descriptor.json.

@tbranyen
Copy link
Member

Left one comment, didn't review the actual generator code, but will trust that @jvilk tested the output.

@tbranyen
Copy link
Member

tbranyen commented Apr 28, 2016

My primary concern here is that we're pushing out code that TypeScript users will rely on and it will need love. Do we have any takers for maintaining?

@joshaber
Copy link
Collaborator

My primary concern here is that we're pushing out code that TypeScript users will rely on and it will need love. Do we have any takers for maintaining?

It's relevant to our interests (/cc https://github.com/github/ohnogit).

@jvilk
Copy link
Author

jvilk commented Apr 28, 2016

One limitation I've noticed: I use PromiseLike as the return value of methods that return promises to avoid dependencies on additional typings or TypeScript's ES6 output mode, which includes typings for native browser Promises. But PromiseLike lacks some of the methods on the promises your library uses. (Example: I think it does not specify catch.)

Due to other commitments, I do not have time to look into this or other issues until mid-May. But I wanted to raise it in case someone else jumps in to take a look at this or improve it.

@johnhaley81
Copy link
Collaborator

I'm mostly interested in a system that automatically lifts the typing out from our docs. That way we could automate everything. There are still issues with some docs but most of them are correct. Also, it provides a large incentive for keeping the docs up-to-date.

This IMO helps with the maintainability of the whole system. Ain't nobody going to want to manually maintain TypeScript typings.

@joshaber
Copy link
Collaborator

Maybe this is nuts, but I almost wonder if we should go the other way: generate docs from a TypeScript declaration. The TypeScript declaration syntax is gonna be more well-defined than any documentation formatting.

We could use this script to seed the declaration, and then going forward treat it as The Truth.

@johnhaley81
Copy link
Collaborator

We could do that. The thing that I did like about the docs is that it's basically jsdoc declarations in the JS code and the rest is lifted from the idefs.json. So really the source of truth is the thing that we generate the C++ code from (plus JS helper methods).

@joshaber
Copy link
Collaborator

Ah right. I forgot it didn't all come from JS.

@pburgmer
Copy link

+1 for this feature. would be create to have these type informations

@maxkorp
Copy link
Collaborator

maxkorp commented Nov 1, 2016

A significant portion of our documentation is incorrect (still directly references the libgit2 docs). Not incorrect enough that one cant manually reference it, but for autogenerating code, I'm not sure. I haven't looked at this closely, but if it's building off of the actual types we build out, that's sane data (as sane as the rest of nodegit, anyways.

@jvilk
Copy link
Author

jvilk commented Nov 1, 2016

This builds off of your JSDoc data, which is sane once I've filtered out some invalid types. I've successfully used the typings produced by this tool in code that interacts with NodeGit.

@tbranyen
Copy link
Member

I feel like this should be moved to the nodegit repo. I'm using TypeScript more now and think this would be great to land. But the documentation site doesn't seem like the right place for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants